@llvm/pr-subscribers-mlir-linalg
@llvm/pr-subscribers-mlir-vector
@llvm/pr-subscribers-flang-openmp
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Maksim Levental (makslevental)

static arith::ConstantIntOp create(OpBuilder& builder, Location location, int64_t value, unsigned width);

diff --git a/mlir/include/mlir/TableGen/Class.h b/mlir/include/mlir/TableGen/Class.h
index f750a34a3b2ba..69cefbbc43e0a 100644
--- a/mlir/include/mlir/TableGen/Class.h
+++ b/mlir/include/mlir/TableGen/Class.h
@@ -71,6 +71,8 @@ class MethodParameter {
   StringRef getName() const { return name; }
   /// Returns true if the parameter has a default value.
   bool hasDefaultValue() const { return !defaultValue.empty(); }
+  StringRef getDefaultValue() const { return defaultValue; }
+  bool isOptional() const { return optional; }
 
 private:
   /// The C++ type.
diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 6008ed4673d1b..65094dcaeb6d8 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -230,6 +230,14 @@ static const char *const opCommentHeader = R"(
 
 )";
 
+static const char *const inlineCreateBody = R"(
+  OperationState __state__({0}, getOperationName());
+  build(builder, __state__, {1});
+  auto __res__ = dyn_cast<{2}>(builder.create(__state__));
+  assert(__res__ && "builder didn't return the right type");
+  return __res__;
+)";
+
 //===----------------------------------------------------------------------===//
 // Utility structs and functions
 //===----------------------------------------------------------------------===//
@@ -665,6 +673,7 @@ class OpEmitter {
   // Generates the build() method that takes each operand/attribute
   // as a stand-alone parameter.
   void genSeparateArgParamBuilder();
+  void genInlineCreateBody(const SmallVector<MethodParameter> &paramList);
 
   // Generates the build() method that takes each operand/attribute as a
   // stand-alone parameter. The generated build() method uses first operand's
@@ -2557,6 +2566,36 @@ static bool canInferType(const Operator &op) {
   return op.getTrait("::mlir::InferTypeOpInterface::Trait");
 }
 
+void OpEmitter::genInlineCreateBody(
+    const SmallVector<MethodParameter> &paramList) {
+  SmallVector<MethodParameter> createParamList;
+  SmallVector<llvm::StringRef, 4> nonBuilderStateArgsList;
+  createParamList.emplace_back("::mlir::OpBuilder &", "builder");
+  std::string locParamName = "loc";
+  while (llvm::find_if(paramList, [&locParamName](const MethodParameter &p) {
+    return p.getName().str() == locParamName;
+  })) {
+    locParamName += "_";
+  }
+  createParamList.emplace_back("::mlir::Location", locParamName);
+
+  for (auto &param : paramList) {
+    if (param.getType() == "::mlir::OpBuilder &" ||
+        param.getType() == "::mlir::OperationState &")
+      continue;
+    createParamList.emplace_back(param.getType(), param.getName(),
+                                 param.getDefaultValue(), param.isOptional());
+    nonBuilderStateArgsList.push_back(param.getName());
+  }
+  auto *c = opClass.addStaticMethod(opClass.getClassName(), "create",
+                                    createParamList);
+  std::string nonBuilderStateArgs = "";
+  llvm::raw_string_ostream nonBuilderStateArgsOS(nonBuilderStateArgs);
+  interleaveComma(nonBuilderStateArgsList, nonBuilderStateArgsOS);
+  c->body() << llvm::formatv(inlineCreateBody, locParamName,
+                             nonBuilderStateArgs, opClass.getClassName());
+}
+
 void OpEmitter::genSeparateArgParamBuilder() {
   SmallVector<AttrParamKind, 2> attrBuilderType;
   attrBuilderType.push_back(AttrParamKind::WrappedAttr);
@@ -2573,10 +2612,12 @@ void OpEmitter::genSeparateArgParamBuilder() {
     buildParamList(paramList, inferredAttributes, resultNames, paramKind,
                    attrType);
 
-    auto *m = opClass.addStaticMethod("void", "build", std::move(paramList));
+    auto *m = opClass.addStaticMethod("void", "build", paramList);
     // If the builder is redundant, skip generating the method.
     if (!m)
       return;
+    genInlineCreateBody(paramList);
+
     auto &body = m->body();
     genCodeForAddingArgAndRegionForBuilder(body, inferredAttributes,
                                            /*isRawValueAttr=*/attrType ==
@@ -2701,10 +2742,11 @@ void OpEmitter::genUseOperandAsResultTypeCollectiveParamBuilder(
   if (op.getNumVariadicRegions())
     paramList.emplace_back("unsigned", "numRegions");
 
-  auto *m = opClass.addStaticMethod("void", "build", std::move(paramList));
+  auto *m = opClass.addStaticMethod("void", "build", paramList);
   // If the builder is redundant, skip generating the method
   if (!m)
     return;
+  genInlineCreateBody(paramList);
   auto &body = m->body();
 
   // Operands
@@ -2815,10 +2857,11 @@ void OpEmitter::genInferredTypeCollectiveParamBuilder(
   if (op.getNumVariadicRegions())
     paramList.emplace_back("unsigned", "numRegions");
 
-  auto *m = opClass.addStaticMethod("void", "build", std::move(paramList));
+  auto *m = opClass.addStaticMethod("void", "build", paramList);
   // If the builder is redundant, skip generating the method
   if (!m)
     return;
+  genInlineCreateBody(paramList);
   auto &body = m->body();
 
   int numResults = op.getNumResults();
@@ -2895,10 +2938,11 @@ void OpEmitter::genUseOperandAsResultTypeSeparateParamBuilder() {
     buildParamList(paramList, inferredAttributes, resultNames,
                    TypeParamKind::None, attrType);
 
-    auto *m = opClass.addStaticMethod("void", "build", std::move(paramList));
+    auto *m = opClass.addStaticMethod("void", "build", paramList);
     // If the builder is redundant, skip generating the method
     if (!m)
       return;
+    genInlineCreateBody(paramList);
     auto &body = m->body();
     genCodeForAddingArgAndRegionForBuilder(body, inferredAttributes,
                                            /*isRawValueAttr=*/attrType ==
@@ -2937,10 +2981,11 @@ void OpEmitter::genUseAttrAsResultTypeCollectiveParamBuilder(
                                  : "attributes";
   paramList.emplace_back("::llvm::ArrayRef<::mlir::NamedAttribute>",
                          attributesName, "{}");
-  auto *m = opClass.addStaticMethod("void", "build", std::move(paramList));
+  auto *m = opClass.addStaticMethod("void", "build", paramList);
   // If the builder is redundant, skip generating the method
   if (!m)
     return;
+  genInlineCreateBody(paramList);
 
   auto &body = m->body();
 
@@ -3103,10 +3148,11 @@ void OpEmitter::genCollectiveParamBuilder(CollectiveBuilderKind kind) {
   if (op.getNumVariadicRegions())
     paramList.emplace_back("unsigned", "numRegions");
 
-  auto *m = opClass.addStaticMethod("void", "build", std::move(paramList));
+  auto *m = opClass.addStaticMethod("void", "build", paramList);
   // If the builder is redundant, skip generating the method
   if (!m)
     return;
+  genInlineCreateBody(paramList);
   auto &body = m->body();
 
   // Operands

Let me ask again the others we previously talked to about adding support SemaCodeComplete side. The cost of adding support may be better trade off than adding additional and migration (I mean the feature clangd side is more useful in general, I have no idea about the cost ... but will ask).

at least upstream

I can send an NFC PR after this one to update all the upstream uses (a regex will be sufficient).

about adding support SemaCodeComplete

1. I don't see how this possible - what if a param pack is forwarded to two calls with different param names? Also parameter pack slicing is possible

2. Is clangd the only auto-complete engine? I don't think so. Even if it were, certainly some IDEs have proprietary builds (eg CLion) and who knows how far behind tip they are.

We would have to deprecate the other form completely everywhere, that's a cost to all users. It can be invoked from template classes/patterns, so a plain regex wouldn't suffice. So prudent to consider alternatives and their timelines.

I'm not proposing to solve this for all possible ways to forward argument templated methods, just those here and we have a fairly standard forwarding. clangd would be main one as its part of this larger project indeed - impossible to cater to all (else we'd probably be using C++11 here still).

Is deprecating the current usage necessary?

Is deprecating the current usage necessary?

Upstream, I would say yes: consistency is important in the codebase IMO: having to completely equivalent way of writing it is hurting readability and accessibility to the codebase.
We don't have to actually deprecate it in the sense of "add a compiler warning that affects all users" and "we will remove this API in a future release". We could just document the old one as "this API exists for historical reasons, we advise people to use Op::create(...) instead".

Here is what the update would look like #147311. It's a lot of files so it would probably be better to chop by affected dir.

Note currently it's missing instances like

arith::ConstantIntOp::build(OpBuilder &builder, OperationState &result, int64_t value, unsigned width)

which are "custom" but these are easily fixed since if they're actually being used now the corresponding arith::ConstantIntOp::create will fail to compile (since it doesn't exist).

Note currently it's missing instances like
arith::ConstantIntOp::build(OpBuilder &builder, OperationState &result, int64_t value, unsigned width)

Oh so I was wrong when I wrote before that:

preserving compatibility with existing builders (important since they can be handwritten today).

I forgot the extra builder leading argument here.
That means that @kuhar's idea is somehow on par with this I think.

How do you write a manual builder now? Ideally we wouldn't add complexity in the body of the builder for users.

Note the removed let builders = are dangling (they have no impls).

Note the removed let builders = are dangling (they have no impls).

Do you mind just pushing this already? This is general cleanup we should lock-in independently of the rest of the work here.

I'm OK with this direction. We need to make a call on what to do about ImplicitLocOpBuilder.h.

Overall I'd like to hear more about this from @jpienaar @ftynse @j2kun (maybe @Mogball and @River707 ?) : are we OK to transition from builder.create<Op>(...) to Op::create(builder, ...) for the benefit of better IDE support?

As far as I understand this is now perfectly backward compatible with existing hand-written build methods, so it can be a scripted mass replacement.

I'm OK with this direction. We need to make a call on what to do about ImplicitLocOpBuilder.h.

Some reasonable options in here I think? https://stackoverflow.com/questions/68306977/how-can-i-deprecate-a-c-header

I am +1 to these types of builder methods. That being said, I am a huge fan of ImplicitLocOpBuilder and would be sad to see it go away :(

I am +1 to these types of builder methods. That being said, I am a huge fan of ImplicitLocOpBuilder and would be sad to see it go away :(

It won't "go away" - it'll just migrate into Builders.h. So in reality it's getting promoted 😃 . The only thing that will go away is the header itself.

I asked the tooling folks, and yes very low chance of any improvement here in foreseeable future (they pointed at how its currently done in one instance, basically it walks and reinterprets functions following params to extract possible types - works only in some cases and requires seeing instantiations).

Overall I think this improves discoverability and is one of the things folks complain about. I prefer the existing format, BUT it is much worse when one doesn't know the args (to the point where I often just navigate to the generated file when unsure). And appears a productivity issue for most. So could get used to it. I think we could do a clang-rewrite for these - there are going to be a lot of code that needs updating, making it easy for users would be great.

It is really nice to be able to define one's own utility builders and that just works still currently. So ImplicitLocOpBuilder one needing to be special cased is a bit unfortunate. Can't think of a better way though (and probably needs to be explicit rather than templated else completion doesn't work again ...).

Oh and I'd be pro merging ImplicitLocOpBuilder in to Builders.h - it is really privileged now. I mean one could just have ImplicitLocOpBuilder.h keep including Builders.h, then you also keep folks happy who get their include indirectly. And then add pragma, soak, delete. The update for others before deletion is then sed s/ImplicitLocOpBuilder.h/Builder.s/ .

Cool glad we've reached consensus :)

I'm a little behind on work-work but I will get this across the finish line soon (early next week at the absolute latest).

Overall I'd like to hear more about this from @jpienaar @ftynse @j2kun (maybe @Mogball and @River707 ?) : are we OK to transition from builder.create(...) to Op::create(builder, ...) for the benefit of better IDE support?

I'd be very happy to transition! I can offer some help if needed.

Strategically, I think this still needs a dedicated forum post for visibility, even though it appears there is consensus in this thread. The post should include instructions for downstreams on how to transition.

I'd also like to deprecate and remove the previous style of builders eventually, following the same consistency arguments as @joker-eph. At the very least for upstream usage. Given the importance of this particular API, I'd say we could deprecate after the next release branches, and remove one or two releases after that. Maybe there'a a way to have a GitHub action check new commits for the create< string and notify about the preferred upstream style as a way of soft deprecation. This is something to discuss on the forum, no particular rush IMO.

are we OK to transition from builder.create(...) to Op::create(builder, ...) for the benefit of better IDE support?

For posterity I'll correct this, @Mogball identified on discourse that is more to it than better IDE support. Not having a template means that we can have user-defined implicit conversion as part of the call.
For example this does not compile today:

  auto acOp = builder.create<spirv::AccessChainOp>(loc, addrOp, {zeroOp, offsetOp});

It throw an error like:

mlir/include/mlir/IR/Builders.h:503:8: note: candidate template ignored: substitution failure [with OpTy = spirv::AccessChainOp]: deduced incomplete pack <mlir::spirv::AddressOfOp &, (no value)> for template parameter 'Args'
  502 |   template <typename OpTy, typename... Args>
      |                                        ~~~~
  503 |   OpTy create(Location location, Args &&...args) {
      |        ^
1 error generated.

We must be explicit and make the type perfectly match:

  auto acOp = builder.create<spirv::AccessChainOp>(loc, addrOp, llvm::ArrayRef({zeroOp, offsetOp}));

Whereas the new form will just work like this:

  auto acOp = spirv::AccessChainOp::create(builder, loc, addrOp, {zeroOp, offsetOp});

I haven't followed completely here, but what is the reason for generating create methods specifically for ImplicitOpBuilder? Is this something that will need to be or should be generalized? I suppose this is to just remove the need for an extra Location argument? Is that right/it?

I haven't followed completely here, but what is the reason for generating create methods specifically for ImplicitOpBuilder?

It's because there are many upstream uses of builder.create<Op>(...) where builder is ImplicitOpBuilder and thus for consistency with the conventional builder.create<Op>(...) I thought it made sense.

This PR now has tests and is ready to land (unless I missed something).

I haven't followed completely here, but what is the reason for generating create methods specifically for ImplicitOpBuilder? Is this something that will need to be or should be generalized? I suppose this is to just remove the need for an extra Location argument? Is that right/it?

It wouldn't be necessary if not to elide the location argument I believe.
That means if you start typing Op::create(builder, and your builder is not an ImplicitLocBuilder you will get the completion only starting with the loc.
Annoyingly I suspect that with an ImplicitLocBuilder it will still offer the overloads with the location argument, since the ImplicitLocBuilder would match as an OpBuilder & as well.